Skip to content

Fix HTTP session timestamp units - #1621

Open
codingkiddo wants to merge 1 commit into
aws:mainfrom
codingkiddo:fix/http-session-epoch-milliseconds
Open

codingkiddo wants to merge 1 commit into
aws:mainfrom
codingkiddo:fix/http-session-epoch-milliseconds

Conversation

@codingkiddo

Copy link
Copy Markdown

Summary

Fix AwsHttpSession timestamps to use epoch milliseconds as required by the Jakarta Servlet HttpSession contract.

Previously, creationTime and lastAccessedTime were stored using Instant#getEpochSecond(), while HttpSession#getCreationTime() and getLastAccessedTime() are defined in milliseconds since the Unix epoch.

Changes

  • Use Instant#toEpochMilli() for session creation time.
  • Use Instant#toEpochMilli() for last accessed time.
  • Convert maxInactiveInterval from seconds to milliseconds when comparing it with session timestamps.
  • Update the existing creation-time test to validate millisecond timestamps.

Testing

Ran:

mvn -pl aws-serverless-java-container-core \
    -Dtest=AwsHttpSessionTest \
    test

and:

mvn -pl aws-serverless-java-container-core clean test

Both pass successfully.

Signed-off-by: Vinod Kumar <codingkiddo@gmail.com>

@aws-sam-tooling-bot aws-sam-tooling-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Results

Reviewed: 75822fe..1ebaab2
Files: 2
Comments: 2


private void touch() {
lastAccessedTime = Instant.now().getEpochSecond();
lastAccessedTime = Instant.now().toEpochMilli();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BUG] Switching touch() to millisecond resolution regresses isNew(), which is implemented as an equality check on the two timestamps (line 121):

public boolean isNew() {
   return lastAccessedTime == creationTime;
}

With second granularity, any getAttribute/setAttribute/removeAttribute call occurring in the same wall-clock second as construction left lastAccessedTime equal to creationTime, so isNew() stayed true for the duration of a typical sub-second Lambda invocation. With toEpochMilli(), the first attribute access almost always advances the timestamp, so isNew() starts returning false within the very same request that created the session.

That contradicts the HttpSession contract: isNew() must return true until the client has joined the session (i.e. until the client sends back the session id on a subsequent request). Since a session here is created per request and never returned by the client, it should remain new for its whole lifetime. Frameworks layered on top (for example Spring Security's session-fixation and "session created" handling) branch on isNew(), so the flip is externally observable.

Decoupling isNew() from the timestamps keeps the fix to units only:

private boolean isNew = true;

@Override
public boolean isNew() {
   return isNew;
}

Note that the existing test validSession_expectCorrectValidationOrInvalidation asserts assertFalse(sess.isNew()) after a Thread.sleep(1000), so it passes either way and does not cover this behavior change.


boolean isValid() {
if (lastAccessedTime - creationTime < maxInactiveInterval) {
if (lastAccessedTime - creationTime < maxInactiveInterval * 1000L) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BUG] The unit conversion here is correct, but it cements a comparison that measures the wrong quantity: lastAccessedTime - creationTime is the session's total age, not its idle time. maxInactiveInterval is defined as the time the container will allow between client requests before invalidating the session, so a continuously used session is invalidated once it is older than the interval, even though it was never idle. Conversely, a session that has been idle for hours is still reported valid as long as it was created recently and never touched again.

The check should compare current time against the last access:

boolean isValid() {
   if (Instant.now().toEpochMilli() - lastAccessedTime < maxInactiveInterval  1000L) {
       return valid;
   } else {
       return false;
   }
}

While touching this line, also consider the spec rule that a zero or negative maxInactiveInterval (settable via setMaxInactiveInterval) means the session never expires. Today a negative value makes the comparison immediately false, so isValid() returns false for a brand-new session — the exact opposite of the intended "never time out" behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant